Skip to content

Fix fetching old access_token from refresh_token #810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Mar 16, 2020
Merged

Fix fetching old access_token from refresh_token #810

merged 2 commits into from
Mar 16, 2020

Conversation

@scaredcat
Copy link
Contributor

Issue

When I scale my back-end to have multiple django instances running to help with growing server demand I sometime receive the following stacktrace error:

ERROR [django.request] [log] [2020-03-10 08:46:47,169] Internal Server Error: /api/oauth2/token/
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/django/db/models/fields/related_descriptors.py", line 164, in __get__
    rel_obj = self.field.get_cached_value(instance)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/fields/mixins.py", line 13, in get_cached_value
    return instance._state.fields_cache[cache_name]
KeyError: 'access_token'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/django/core/handlers/exception.py", line 34, in inner
    response = get_response(request)
  File "/usr/local/lib/python3.7/site-packages/django/core/handlers/base.py", line 115, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/usr/local/lib/python3.7/site-packages/django/core/handlers/base.py", line 113, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/usr/local/lib/python3.7/site-packages/django/views/generic/base.py", line 71, in view
    return self.dispatch(request, *args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/django/utils/decorators.py", line 45, in _wrapper
    return bound_method(*args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
    return view_func(*args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/newrelic/hooks/framework_django.py", line 885, in wrapper
    return wrapped(*args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/django/views/generic/base.py", line 97, in dispatch
    return handler(request, *args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/django/utils/decorators.py", line 45, in _wrapper
    return bound_method(*args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/django/views/decorators/debug.py", line 76, in sensitive_post_parameters_wrapper
    return view(request, *args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/oauth2_provider/views/base.py", line 206, in post
    url, headers, body, status = self.create_token_response(request)
  File "/usr/local/lib/python3.7/site-packages/oauth2_provider/views/mixins.py", line 123, in create_token_response
    return core.create_token_response(request)
  File "/usr/local/lib/python3.7/site-packages/oauthlib/oauth2/rfc6749/endpoints/base.py", line 116, in wrapper
    return f(endpoint, uri, *args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/oauthlib/oauth2/rfc6749/endpoints/token.py", line 119, in create_token_response
    request, self.default_token_type)
  File "/usr/local/lib/python3.7/site-packages/oauthlib/oauth2/rfc6749/grant_types/refresh_token.py", line 60, in create_token_response
    self.validate_token_request(request)
  File "/usr/local/lib/python3.7/site-packages/oauthlib/oauth2/rfc6749/grant_types/refresh_token.py", line 125, in validate_token_request
    request.refresh_token, request))
  File "/usr/local/lib/python3.7/site-packages/oauth2_provider/oauth2_validators.py", line 607, in get_original_scopes
    return rt.access_token.scope
  File "/usr/local/lib/python3.7/site-packages/django/db/models/fields/related_descriptors.py", line 178, in __get__
    rel_obj = self.get_object(instance)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/fields/related_descriptors.py", line 298, in get_object
    return super().get_object(instance)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/fields/related_descriptors.py", line 145, in get_object
    return qs.get(self.field.get_reverse_related_filter(instance))
  File "/usr/local/lib/python3.7/site-packages/django/db/models/query.py", line 408, in get
    self.model._meta.object_name
oauth2_provider.models.AccessToken.DoesNotExist: AccessToken matching query does not exist.

This is due to multiple refresh_token requests being sent from the same client to /api/oauth2/token/ where the requests are handled in parallel by the back-end.

  1. oauthlib tries to validate the refresh token
    https://github.com/oauthlib/oauthlib/blob/b71636e85f845b79b5a56de6480fcba9d1415720/oauthlib/oauth2/rfc6749/grant_types/refresh_token.py#L58
    which then calls the validate_refresh_token in djago-oauth-toolkit
    https://github.com/oauthlib/oauthlib/blob/b71636e85f845b79b5a56de6480fcba9d1415720/oauthlib/oauth2/rfc6749/grant_types/refresh_token.py#L115-L116

  2. The refresh_token is read from the database and attached to the request object
    https://github.com/jazzband/django-oauth-toolkit/blob/eb381c30affa60a0465a8a2f2c5ca0890ece5362/oauth2_provider/oauth2_validators.py#L637-L642

  3. It then calls get_original_scopes
    https://github.com/oauthlib/oauthlib/blob/b71636e85f845b79b5a56de6480fcba9d1415720/oauthlib/oauth2/rfc6749/grant_types/refresh_token.py#L122-L123
    which in django-oauth-toolkit reads the access_token from the refresh_token this requires a db lookup
    https://github.com/jazzband/django-oauth-toolkit/blob/eb381c30affa60a0465a8a2f2c5ca0890ece5362/oauth2_provider/oauth2_validators.py#L624

  4. later oauthlib saves the token
    https://github.com/oauthlib/oauthlib/blob/b71636e85f845b79b5a56de6480fcba9d1415720/oauthlib/oauth2/rfc6749/grant_types/refresh_token.py#L70
    which eventually calls save_bearer_token
    https://github.com/oauthlib/oauthlib/blob/b71636e85f845b79b5a56de6480fcba9d1415720/oauthlib/oauth2/rfc6749/request_validator.py#L315
    and then revoke() the refresh_token
    https://github.com/jazzband/django-oauth-toolkit/blob/eb381c30affa60a0465a8a2f2c5ca0890ece5362/oauth2_provider/oauth2_validators.py#L532
    which in tern deletes the access token.

If you have two servers you can have
Server 1: (1) -> (2) -> (3) -> (4)
Server 2: (1) -> (2) ---------------> (3) -> (4)

Notice server 2 only gets to check the refresh_token after server 1 has already revoked (deleted) the access_token that is attached to the refresh_token causing the error on server 2 (stack-trace) above.

Description of the Change

Ensure that a database lookup is not required to get the access_token from the refresh_token in case another parallel refresh request has already removed that access_token from the database

@coveralls
Copy link

coveralls commented Mar 13, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 1331

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.768%
Totals Coverage Status
Change from base Build 1330: 0.0%
Covered Lines: 1268
Relevant Lines: 1338
💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1322

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.768%
Totals Coverage Status
Change from base Build 1320: 0.0%
Covered Lines: 1268
Relevant Lines: 1338
💛 - Coveralls

@scaredcat scaredcat force-pushed the fix-key-error-concurrency branch from b0ce48f to a37d77d Compare March 13, 2020 03:51
@auvipy auvipy merged commit 6b2f5f8 into django-oauth:master Mar 16, 2020
n2ygk
n2ygk reviewed Mar 16, 2020
View reviewed changes
Copy link
Contributor

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scaredcat Please create a new PR with a CHANGELOG entry documenting there's a bug fix. I think that qualifies as a user-relevant change. Thanks!

scaredcat added a commit to scaredcat/django-oauth-toolkit that referenced this pull request Mar 16, 2020
@scaredcat
Add changelog for PR django-oauth#810
585c551
@scaredcat scaredcat mentioned this pull request Mar 16, 2020
Merged
@scaredcat
Copy link
Contributor Author

@scaredcat Please create a new PR with a CHANGELOG entry documenting there's a bug fix. I think that qualifies as a user-relevant change. Thanks!

#813

auvipy pushed a commit that referenced this pull request Mar 17, 2020
@scaredcat @auvipy
Add changelog for PR #810
fd0a0d8
This was referenced Apr 6, 2020
Open
Open
This was referenced Mar 15, 2021
Closed
Closed
Closed
Closed
Closed
@obulat obulat mentioned this pull request Apr 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@n2ygk n2ygk n2ygk left review comments

+1 more reviewer

@auvipy auvipy auvipy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@scaredcat @coveralls @n2ygk @auvipy